Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: relax auth test expiration window for CIs under high contention #995

Merged
merged 4 commits into from
Oct 31, 2023

Conversation

pratik151192
Copy link
Contributor

@pratik151192 pratik151192 commented Oct 31, 2023

For https://github.com/momentohq/dev-eco-issue-tracker/issues/536

I have been observing failed CIs on this repo since a while and the most common culprits in my observation have been:

  • 9/10 times it was the test in this PR that failed. The test essentially checks that the expiration duration of a token generated at time x is within y-1 and y+1 where y is the expiration time returned by the server after the API call. The window here is small even though it does succeed locally in a loop of 1000 runs. I believe the reason it fails on CIs so frequently is either:
    • Event loop contention
    • Server side delay that might happen due to several things.
    • In one of my failed CIs, the drift was as high as 23 seconds
generate auth token using session token credentials › should succeed for generating an api token that expires

    expect(received).toBeWithin(expected)

    Expected number to be within start (inclusive) and end (exclusive):
      start: 1698452568  end: 1698452571
    Received:
      1698452594

      79 |       expect(expireResponseSuccess.is_success);
      80 |       expect(expireResponseSuccess.expiresAt.doesExpire());
    > 81 |       expect(expireResponseSuccess.expiresAt.epoch()).toBeWithin(
         |                                                       ^
      82 |         expiresIn - 1,
      83 |         expiresIn + 2
      84 |       );

Instead of testing expirationTime at a second granularity with 1 second error margin, I have changed it to test at minute granularity with 1 minute error margin. This gives some breathing room for the CIs to succeed.

  • The remaining errors i have seen are due to client side timeouts. I have increased the timeout to 60 seconds from 5 seconds for similar reasons of event loop contention (just a theory).

@pratik151192 pratik151192 marked this pull request as ready for review October 31, 2023 17:50
Copy link
Contributor

@anitarua anitarua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, ty!!

@pratik151192 pratik151192 merged commit 411b295 into main Oct 31, 2023
13 checks passed
@pratik151192 pratik151192 deleted the relax-auth-tests branch October 31, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants